tests/Usage/UsageBase.php (2)

67-72: Potential ClickHouse flakiness due to eventual consistency.

Similar to testGetByPeriod, ClickHouse's SummingMergeTree merges rows asynchronously. The assertion on line 71 may intermittently fail if rows haven't merged yet when running against ClickHouse.

Consider adding eventual consistency handling (e.g., retry loop with timeout) for ClickHouse adapter tests, consistent with fixes applied to other tests in this trait.

---

150-167: Consider reducing sleep duration or documenting necessity.

The test uses sleep(2) twice, adding 4 seconds to the test suite runtime. While some delay may be necessary for time-based purge testing, consider:

1. Reducing the sleep duration if shorter intervals suffice
2. Adding a brief comment explaining why these sleeps are required

src/Usage/Usage.php (1)

60-63: Use constant for default period value.

The default parameter uses the string literal '1h' instead of self::PERIOD_1H. This creates an inconsistency since the constant is defined and used elsewhere.

Suggested fix

-    public function log(string $metric, int $value, string $period = '1h', array $tags = []): bool
+    public function log(string $metric, int $value, string $period = self::PERIOD_1H, array $tags = []): bool

tests/Usage/UsageBase.php (1)

150-167: Consider reducing sleep duration or using polling for faster tests.

The test uses sleep(2) twice (4 seconds total), making it slow. Consider reducing the sleep durations or implementing a retry/polling mechanism to speed up the test suite while still ensuring the purge behavior is validated.

Potential improvement

     public function testPurge(): void
     {
-        sleep(2);
+        sleep(1);

         // Add a metric
         $this->usage->log('purge-test', 999, '1h');

         // Wait a bit
-        sleep(2);
+        sleep(1);

         // Purge all metrics
         $status = $this->usage->purge(DateTime::now());

src/Usage/Adapter/ClickHouse.php (1)

424-427: Nullable tenant column with allow_nullable_key=1 is functional but suboptimal.

The tenant column is Nullable(UInt64) and used in ORDER BY with allow_nullable_key = 1 setting. While this works, using a non-nullable type with a sentinel default (e.g., UInt64 DEFAULT 0) would be more performant for ClickHouse query optimization.

Also applies to: 450-459

src/Usage/Adapter/Database.php (2)

55-59: Suppress unused parameter warning.

The $id parameter is required by the abstract SQL parent class but unused in this adapter. Add a suppression annotation to silence the static analysis warning.

Suggested fix

     /**
      * Get column definition for Database adapter (not used, but required by SQL parent)
+     *
+     * `@SuppressWarnings`(PHPMD.UnusedFormalParameter)
      */
     protected function getColumnDefinition(string $id): string
     {

---

270-281: Inefficient sum implementation fetches all rows.

sumByPeriod retrieves all matching documents and iterates in PHP to compute the sum. For large datasets, this is memory-intensive. Consider using database-level aggregation if Utopia\Database supports it, or document this limitation.

Consider using database sum if available

// If Utopia\Database supports sum aggregation:
// return $this->db->sum($this->collection, 'value', $dbQueries);

// Otherwise, document the limitation and consider pagination
// for very large result sets to avoid memory issues

src/Usage/Query.php (1)

5-10: Update class docblock - not just for ClickHouse.

The docblock states "Query class for ClickHouse Audit adapter" but this class is used by both ClickHouse and Database adapters. Update the description to reflect its general purpose.

Suggested fix

 /**
- * Query class for ClickHouse Audit adapter
+ * Query class for Usage adapters
  *
- * Provides a fluent interface for building ClickHouse queries.
- * Contains only methods needed for audit log operations.
+ * Provides a fluent interface for building usage metric queries.
+ * Used by all adapter implementations (Database, ClickHouse, etc.)
  */

src/Usage/Adapter/Database.php (1)

52-59: Silence unused parameter warning.

PHPMD flags $id as unused; consider renaming to $_id or suppressing for clarity.

src/Usage/Adapter/ClickHouse.php (2)

601-604: Redundant type check.

The $tags parameter has type hint array (line 581), so is_array($tags) is always true. This defensive check is harmless but unnecessary.

---

1141-1160: Simplify redundant is_array checks.

Query::getValues() always returns an array, making the is_array($values) checks redundant. The current structure is confusing because $singleValue is assigned conditionally but used unconditionally.

♻️ Cleaner implementation

                 case Query::TYPE_LESSER_EQUAL:
                     $this->validateAttributeName($attribute);
                     $escapedAttr = $this->escapeIdentifier($attribute);
                     $paramName = 'param_' . $paramCounter++;
+                    $singleValue = $values[0] ?? null;
                     if ($attribute === 'time') {
-                        if (is_array($values)) {
-                            /** `@var` \DateTime|string|null $singleValue */
-                            $singleValue = $values[0] ?? null;
-                        }
                         $filters[] = "{$escapedAttr} <= {{$paramName}:DateTime64(3)}";
                         $params[$paramName] = $this->formatDateTime($singleValue);
                     } else {
-                        if (is_array($values)) {
-                            /** `@var` bool|float|int|string $singleValue */
-                            $singleValue = $values[0] ?? null;
-                        }
                         $filters[] = "{$escapedAttr} <= {{$paramName}:String}";
                         $params[$paramName] = $this->formatParamValue($singleValue);
                     }
                     break;

src/Usage/Adapter/Database.php (1)

52-58: Silence PHPMD unused-parameter warning for required override.

getColumnDefinition(string $id) is required by the abstract base but not used here. Add a suppression to avoid noisy analysis warnings.

🧹 Suggested tweak

-    /**
-     * Get column definition for Database adapter (not used, but required by SQL parent)
-     */
+    /**
+     * Get column definition for Database adapter (not used, but required by SQL parent)
+     *
+     * `@SuppressWarnings`(PHPMD.UnusedFormalParameter)
+     */
     protected function getColumnDefinition(string $id): string

tests/Usage/UsageBase.php (2)

302-330: Remove unused variables $aggCount and $counterCount.

These variables are assigned but never used in assertions. Either remove them or add assertions to verify the count behavior matches expectations.

Proposed fix

         $aggSum = $this->usage->sumByPeriod('agg-vs-counter', '1h');
-        $aggCount = $this->usage->countByPeriod('agg-vs-counter', '1h');
 
         // Clear for counter test
         $this->usage->purge(DateTime::now());
 
         // Log same counter metric 3 times (last one wins)
         $this->assertTrue($this->usage->logCounter('counter-vs-agg', 10, '1h', []));
         $this->assertTrue($this->usage->logCounter('counter-vs-agg', 20, '1h', []));
         $this->assertTrue($this->usage->logCounter('counter-vs-agg', 30, '1h', []));
 
         $counterSum = $this->usage->sumByPeriod('counter-vs-agg', '1h');
-        $counterCount = $this->usage->countByPeriod('counter-vs-agg', '1h');

---

173-195: Assertions always pass - consider strengthening.

Both testLessThanEqualQuery and testGreaterThanEqualQuery use assertGreaterThanOrEqual(0, count($results)) which always passes. If the intent is only to verify no exceptions are thrown, consider using assertIsArray($results) instead for clarity.

src/Usage/Adapter/Database.php (2)

93-140: $batchSize parameter is unused - consider implementing or documenting.

The logBatch method ignores the $batchSize parameter and processes all metrics in a single upsert operation. While this may be acceptable for the Database adapter, it differs from the ClickHouse adapter which respects the batch size for chunked inserts. Consider either:

1. Implementing batch chunking for consistency
2. Adding a comment explaining why batching isn't needed for this adapter

---

388-407: Consider using batch deletion for better performance.

The purge method deletes documents one-by-one in a loop. For large datasets, this could be slow. Consider using a batch deletion approach if the underlying database supports it.

src/Usage/Adapter/Database.php (3)

93-93: Unused $batchSize parameter.

The $batchSize parameter is declared but never used. Either implement batching logic to process documents in chunks of $batchSize, or remove the parameter to avoid confusion.

Option 1: Implement batching

     public function logBatch(array $metrics, int $batchSize = 1000): bool
     {
         $this->db->getAuthorization()->skip(function () use ($metrics, $batchSize) {
             $documentsById = [];
             // ... existing aggregation logic ...
 
-            if (!empty($documents)) {
-                $this->db->upsertDocumentsWithIncrease($this->collection, 'value', $documents);
-            }
+            if (!empty($documents)) {
+                foreach (array_chunk($documents, $batchSize) as $batch) {
+                    $this->db->upsertDocumentsWithIncrease($this->collection, 'value', $batch);
+                }
+            }
         });

---

188-188: Unused $batchSize parameter (same as logBatch).

Apply the same batching fix as suggested for logBatch(), or remove the parameter.

---

388-407: Purge implementation is functional but could be optimized.

The current approach deletes documents one-by-one in a loop. While this works correctly, it may be slow for large datasets. Consider using a bulk delete operation if available in the Utopia\Database library.

src/Usage/Adapter/ClickHouse.php (2)

1243-1256: Simplify value extraction — $singleValue only assigned conditionally but used unconditionally.

Query::getValues() always returns an array, making is_array($values) always true. However, $singleValue is only assigned inside the if (is_array($values)) block but used unconditionally at line 1248. This creates fragile code structure.

♻️ Proposed refactor

                 case Query::TYPE_LESSER_EQUAL:
                     $this->validateAttributeName($attribute);
                     $escapedAttr = $this->escapeIdentifier($attribute);
                     $paramName = 'param_' . $paramCounter++;
+                    $singleValue = $values[0] ?? null;
                     if ($attribute === 'time') {
-                        if (is_array($values)) {
-                            /** `@var` \DateTime|string|null $singleValue */
-                            $singleValue = $values[0] ?? null;
-                        }
                         $filters[] = "{$escapedAttr} <= {{$paramName}:DateTime64(3)}";
                         $params[$paramName] = $this->formatDateTime($singleValue);
                     } else {
-                        if (is_array($values)) {
-                            /** `@var` bool|float|int|string $singleValue */
-                            $singleValue = $values[0] ?? null;
-                        }
                         $filters[] = "{$escapedAttr} <= {{$paramName}:String}";
                         $params[$paramName] = $this->formatParamValue($singleValue);
                     }
                     break;

---

1264-1277: Same refactor needed for TYPE_GREATER_EQUAL.

Same issue as TYPE_LESSER_EQUAL — $singleValue is conditionally assigned but unconditionally used.

♻️ Proposed refactor

                 case Query::TYPE_GREATER_EQUAL:
                     $this->validateAttributeName($attribute);
                     $escapedAttr = $this->escapeIdentifier($attribute);
                     $paramName = 'param_' . $paramCounter++;
+                    $singleValue = $values[0] ?? null;
                     if ($attribute === 'time') {
-                        if (is_array($values)) {
-                            /** `@var` \DateTime|string|null $singleValue */
-                            $singleValue = $values[0] ?? null;
-                        }
                         $filters[] = "{$escapedAttr} >= {{$paramName}:DateTime64(3)}";
                         $params[$paramName] = $this->formatDateTime($singleValue);
                     } else {
-                        if (is_array($values)) {
-                            /** `@var` bool|float|int|string $singleValue */
-                            $singleValue = $values[0] ?? null;
-                        }
                         $filters[] = "{$escapedAttr} >= {{$paramName}:String}";
                         $params[$paramName] = $this->formatParamValue($singleValue);
                     }
                     break;